Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-39333: [C++] Don't use "if constexpr" in lambda #39334

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

kou
Copy link
Member

@kou kou commented Dec 21, 2023

Rationale for this change

It seems that it's not portable. At least it doesn't work as expected with Visual Studio 2017:

C:/arrow/cpp/src/arrow/array/array_nested.cc(291): error C2065: 'validity': undeclared identifier (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx) [C:\arrow-build\src\arrow\arrow_shared.vcxproj]
  C:/arrow/cpp/src/arrow/array/array_nested.cc(660): note: see reference to function template instantiation 'arrow::Result<std::shared_ptr<arrow::Array>> arrow::`anonymous-namespace'::FlattenListViewArray<arrow::ListViewArray,false>(const ListViewArrayT &,arrow::MemoryPool *)' being compiled
          with
          [
              ListViewArrayT=arrow::ListViewArray
          ] (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx)
  memory_pool.cc
C:/arrow/cpp/src/arrow/array/array_nested.cc(291): error C2065: 'list_view_array_offset': undeclared identifier (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx) [C:\arrow-build\src\arrow\arrow_shared.vcxproj]

What changes are included in this PR?

Avoid "if constexpr" in lambda.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@kou
Copy link
Member Author

kou commented Dec 21, 2023

@github-actions crossbow submit wheel-windows-*

Copy link

⚠️ GitHub issue #39333 has been automatically assigned in GitHub to PR creator.

This comment was marked as outdated.

@kou kou force-pushed the cpp-list-view-if-constexpr branch from 48321ef to 2957d9b Compare December 21, 2023 02:41
It seems that it's not portable. At least it doesn't work as expected
with Visual Studio 2017:

    C:/arrow/cpp/src/arrow/array/array_nested.cc(291): error C2065: 'validity': undeclared identifier (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx) [C:\arrow-build\src\arrow\arrow_shared.vcxproj]
      C:/arrow/cpp/src/arrow/array/array_nested.cc(660): note: see reference to function template instantiation 'arrow::Result<std::shared_ptr<arrow::Array>> arrow::`anonymous-namespace'::FlattenListViewArray<arrow::ListViewArray,false>(const ListViewArrayT &,arrow::MemoryPool *)' being compiled
              with
              [
                  ListViewArrayT=arrow::ListViewArray
              ] (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx)
      memory_pool.cc
    C:/arrow/cpp/src/arrow/array/array_nested.cc(291): error C2065: 'list_view_array_offset': undeclared identifier (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx) [C:\arrow-build\src\arrow\arrow_shared.vcxproj]

Use "if constexpr" in outer lambda.
@kou kou force-pushed the cpp-list-view-if-constexpr branch from 2957d9b to 47ed583 Compare December 21, 2023 02:53
@kou
Copy link
Member Author

kou commented Dec 21, 2023

@github-actions crossbow submit wheel-windows-*

Copy link

Revision: 47ed583

Submitted crossbow builds: ursacomputing/crossbow @ actions-516f35e778

Task Status
wheel-windows-cp310-amd64 GitHub Actions
wheel-windows-cp311-amd64 GitHub Actions
wheel-windows-cp312-amd64 GitHub Actions
wheel-windows-cp38-amd64 GitHub Actions
wheel-windows-cp39-amd64 GitHub Actions

@kou kou requested a review from felipecrv December 21, 2023 03:34
@kou
Copy link
Member Author

kou commented Dec 21, 2023

@felipecrv This is happen after #35345 is merged.

const auto* offsets = list_view_array.data()->template GetValues<offset_type>(1);
const auto* sizes = list_view_array.data()->template GetValues<offset_type>(2);

auto is_null_or_empty = [&](int64_t i) {
if constexpr (HasNulls) {
std::function<bool(int64_t)> is_null_or_empty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is complicated. Instead, how about just:

  auto is_null_or_empty = [&](int64_t i) {
    if (HasNulls && !bit_util::GetBit(validity, list_view_array_offset + i)) {
      return true;
    }
    return sizes[i] == 0;
  };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1.

The compiler will constant-fold it when the lambda gets inlined. Putting it in std::function would prevent inlining.

@pitrou
Copy link
Member

pitrou commented Dec 21, 2023

@github-actions crossbow submit -g cpp wheel-windows-*

Copy link

Revision: e2ee3d3

Submitted crossbow builds: ursacomputing/crossbow @ actions-0dda7a7461

Task Status
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind Azure
test-cuda-cpp GitHub Actions
test-debian-11-cpp-amd64 GitHub Actions
test-debian-11-cpp-i386 GitHub Actions
test-fedora-38-cpp GitHub Actions
test-ubuntu-20.04-cpp GitHub Actions
test-ubuntu-20.04-cpp-bundled GitHub Actions
test-ubuntu-20.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-20.04-cpp-thread-sanitizer GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
wheel-windows-cp310-amd64 GitHub Actions
wheel-windows-cp311-amd64 GitHub Actions
wheel-windows-cp312-amd64 GitHub Actions
wheel-windows-cp38-amd64 GitHub Actions
wheel-windows-cp39-amd64 GitHub Actions

@pitrou pitrou merged commit 5df541d into apache:main Dec 21, 2023
34 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Dec 21, 2023
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Dec 21, 2023
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 5df541d.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.

@kou kou deleted the cpp-list-view-if-constexpr branch December 22, 2023 01:03
clayburn pushed a commit to clayburn/arrow that referenced this pull request Jan 23, 2024
### Rationale for this change

It seems that it's not portable. At least it doesn't work as expected with Visual Studio 2017:

    C:/arrow/cpp/src/arrow/array/array_nested.cc(291): error C2065: 'validity': undeclared identifier (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx) [C:\arrow-build\src\arrow\arrow_shared.vcxproj]
      C:/arrow/cpp/src/arrow/array/array_nested.cc(660): note: see reference to function template instantiation 'arrow::Result<std::shared_ptr<arrow::Array>> arrow::`anonymous-namespace'::FlattenListViewArray<arrow::ListViewArray,false>(const ListViewArrayT &,arrow::MemoryPool *)' being compiled
              with
              [
                  ListViewArrayT=arrow::ListViewArray
              ] (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx)
      memory_pool.cc
    C:/arrow/cpp/src/arrow/array/array_nested.cc(291): error C2065: 'list_view_array_offset': undeclared identifier (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx) [C:\arrow-build\src\arrow\arrow_shared.vcxproj]
### What changes are included in this PR?

Avoid "if constexpr" in lambda.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* Closes: apache#39333

Lead-authored-by: Antoine Pitrou <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
### Rationale for this change

It seems that it's not portable. At least it doesn't work as expected with Visual Studio 2017:

    C:/arrow/cpp/src/arrow/array/array_nested.cc(291): error C2065: 'validity': undeclared identifier (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx) [C:\arrow-build\src\arrow\arrow_shared.vcxproj]
      C:/arrow/cpp/src/arrow/array/array_nested.cc(660): note: see reference to function template instantiation 'arrow::Result<std::shared_ptr<arrow::Array>> arrow::`anonymous-namespace'::FlattenListViewArray<arrow::ListViewArray,false>(const ListViewArrayT &,arrow::MemoryPool *)' being compiled
              with
              [
                  ListViewArrayT=arrow::ListViewArray
              ] (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx)
      memory_pool.cc
    C:/arrow/cpp/src/arrow/array/array_nested.cc(291): error C2065: 'list_view_array_offset': undeclared identifier (compiling source file C:\arrow-build\src\arrow\CMakeFiles\arrow_shared.dir\Unity\unity_0_cxx.cxx) [C:\arrow-build\src\arrow\arrow_shared.vcxproj]
### What changes are included in this PR?

Avoid "if constexpr" in lambda.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* Closes: apache#39333

Lead-authored-by: Antoine Pitrou <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI][Python][Packaging] Failed to build wheel for Windows since ListView merge
3 participants